Skip to content

Conversation

@Dwij1704
Copy link
Member

@Dwij1704 Dwij1704 commented May 8, 2025

📥 Pull Request

📘 Description
Update print logger to log to buffer and console, and modify cleanup and upload functions to handle buffer content.
🧪 Testing
Describe the tests you performed to validate your changes.

@Dwij1704 Dwij1704 requested review from bboynton97, Copilot and tcdent May 8, 2025 08:44
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the logging mechanism by replacing file-based logging with an in-memory buffer that logs to both the console and memory.

  • Replaces file logging with a StringIO-based memory buffer
  • Updates cleanup and upload functions to clear and use the in-memory log content
Comments suppressed due to low confidence (2)

agentops/logging/instrument_logging.py:18

  • [nitpick] The variable 'file_logger' is misleading in the context of in-memory logging. Consider renaming it to 'in_memory_logger' for better clarity.
file_logger = logging.getLogger('agentops_file_logger')

agentops/logging/instrument_logging.py:4

  • The 'atexit' module is imported but not used. Remove this unused import to clean up the code.
import atexit

@Dwij1704 Dwij1704 requested a review from Copilot May 8, 2025 08:49
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors the logging functionality to use an in-memory buffer instead of a file, updating both the logging setup and cleanup mechanisms, and modifying the log upload process.

  • Switch from file-based logging to an in-memory buffer
  • Update cleanup to clear the in-memory buffer and restore the original print function
  • Modify log upload to read and then clear the buffer content

…riginal print function is only replaced once.
Copy link
Contributor

@bboynton97 bboynton97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i want to test on my machine today but the code looks good

@bboynton97
Copy link
Contributor

tests need updated to account for the new logging method

Copy link
Contributor

@tcdent tcdent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get itttt

@Dwij1704 Dwij1704 merged commit e1bab57 into main May 12, 2025
7 of 9 checks passed
@Dwij1704 Dwij1704 deleted the refactor-logs branch May 12, 2025 21:17
@codecov
Copy link

codecov bot commented May 12, 2025

Codecov Report

Attention: Patch coverage is 80.95238% with 4 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
agentops/logging/instrument_logging.py 80.95% 4 Missing ⚠️

📢 Thoughts on this report? Let us know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants